Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: mobile menu #129

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

zerkalica
Copy link
Contributor

No description provided.

@zerkalica zerkalica force-pushed the feature/mobile-menu branch 2 times, most recently from 3d1b4dd to dbb1723 Compare March 20, 2019 22:49
shouldForwardProp: prop => prop !== 'active',
})<HorizontalMenuItemProps>(({active, theme}) => ({
cursor: 'pointer',
fontFamily: theme.font.family,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

типографику через Typo примитив или Heading, Paragraph или Text компоненты

Copy link
Contributor Author

@zerkalica zerkalica Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У меня ссылка, надо pointer, hover задавать и т.д. Heading, Paragraph, Text - обычные компоненты, у которых нужные свойства не переопределить.

Что лучше,

  1. Сделать один семантический styled компонент, не наследуясь, со всеми стилизациями в одном месте (как сейчас)? Пропсов ему задавать не надо.

  2. По аналогии с Text сделать Link на теге Typo, часть стилей захреначить через пропсы, а часть через inline css? Как тут понять, что хардкодить в стилях, а что в пропсы?

  3. унаследоваться от Typo, часть стилей захреначить через пропсы, часть захардкодить в styled?

Меня смущает, что в компоненте в 2, 3 стилизация делается 2мя способами: пропсы и стили, цельность хуже, лучше что-то одно.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пока оставил, до готовности LinkControl

const levels = new MenuLevelBuilder(items).levels(activeId)
const level0 = levels[0]
const level1 = levels[1]
const level2 = levels[2]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

только два подменю может быть?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В дизайне одно подменю. Делал 2, что б протестить, могу убрать level2. Каждый уровень может быть кастомный, рисовать в цикле не вариант.

/**
* Active menu item id
*/
activeId: string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

я бы selected назвал

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Переименовал

id: string
activeId: string
items: Item[]
onClick: (item: Item) => void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

наверное onSelect или onChange лучше

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сделал onSelect

@dimonka83 dimonka83 changed the title Feature/mobile menu 🚧 feat: mobile menu Apr 10, 2019
@zerkalica zerkalica force-pushed the feature/mobile-menu branch from 8fd2384 to c6b0320 Compare April 16, 2019 16:02
onMouseUp={renderProps.onMouseUp}
onMouseDown={renderProps.onMouseDown}
cursor="pointer"
mr={props.isLast ? 0 : 6}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

отступы лучше во врапере сделать

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

renderProps.hover || props.active ? '#ff8c00' : 'transparent'
}`}
children={
<Typo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

может быть какой-нить Text заюзать?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

заюзал

zerkalica pushed a commit to zerkalica/pijma that referenced this pull request Apr 22, 2019
zerkalica pushed a commit to zerkalica/pijma that referenced this pull request Apr 22, 2019
@zerkalica zerkalica force-pushed the feature/mobile-menu branch from 1bb1c92 to b0455ae Compare April 22, 2019 13:39
zerkalica pushed a commit to zerkalica/pijma that referenced this pull request Apr 22, 2019
@zerkalica zerkalica force-pushed the feature/mobile-menu branch from b0455ae to 4c73ad2 Compare April 22, 2019 13:57
@dimonka83 dimonka83 marked this pull request as draft August 3, 2022 04:40
@dimonka83 dimonka83 changed the title 🚧 feat: mobile menu feat: mobile menu Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants